Re: [PATCH 1/5] OvmfPkg: Handle Cloud Hypervisor host bridge

["Gerd Hoffmann" <kraxel@redhat.com>]

  Hi,

> +    case CLOUDHV_DEVICE_ID: // Cloud Hypervisor host bridge

No need for the comment ...

> +++ b/OvmfPkg/Include/IndustryStandard/CloudHv.h
> @@ -0,0 +1,35 @@
> +/** @file
> +  Various defines related to Cloud Hypervisor
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +#ifndef __CLOUDHV_H__
> +#define __CLOUDHV_H__
> +
> +//
> +// Host Bridge Device ID
> +//
> +#define CLOUDHV_DEVICE_ID 0x0d57

... this is enough documentation IMHO.

> +  if (mAcpiPmBaseAddress == 0) {
> +    IoWrite8 (CLOUDHV_ACPI_SHUTDOWN_IO_ADDRESS, 5 << 2 | 1 << 5);

This looks like the sleep control register of the hw-reduced acpi pm
profile.  I'd suggest to use a new variable then, for example use

    mAcpiHwReducedSleepCtl = CLOUDHV_ACPI_SHUTDOWN_IO_ADDRESS

> +    case 0xffff: /* microvm */
> +      return;

Huh?

> @@ -778,21 +782,22 @@ QemuInitializeRam (
>    if (IsMtrrSupported ()) {
>      MtrrGetAllMtrrs (&MtrrSettings);
>  
> -    //
> -    // MTRRs disabled, fixed MTRRs disabled, default type is uncached
> -    //
> -    ASSERT ((MtrrSettings.MtrrDefType & BIT11) == 0);
> -    ASSERT ((MtrrSettings.MtrrDefType & BIT10) == 0);
> -    ASSERT ((MtrrSettings.MtrrDefType & 0xFF) == 0);
> -
> -    //
> -    // flip default type to writeback
> -    //
> -    SetMem (&MtrrSettings.Fixed, sizeof MtrrSettings.Fixed, 0x06);
> -    ZeroMem (&MtrrSettings.Variables, sizeof MtrrSettings.Variables);
> -    MtrrSettings.MtrrDefType |= BIT11 | BIT10 | 6;
> -    MtrrSetAllMtrrs (&MtrrSettings);
> +    if (mHostBridgeDevId != CLOUDHV_DEVICE_ID) {

Do you need the MtrrGetAllMtrrs() call?  If not you can just use this:

   if (IsMtrrSupported () && mHostBridgeDevId != CLOUDHV_DEVICE_ID) 


> @@ -372,14 +375,18 @@ MiscInitialization (
>                               MICROVM_PSEUDO_DEVICE_ID);
>        ASSERT_RETURN_ERROR (PcdStatus);
>        return;
> +    case CLOUDHV_DEVICE_ID:
> +      DEBUG ((DEBUG_INFO, "%a: Cloud Hypervisor host bridge\n", __FUNCTION__));
> +      PcdStatus = PcdSet16S (PcdOvmfHostBridgePciDevId,
> +                             CLOUDHV_DEVICE_ID);
> +      ASSERT_RETURN_ERROR (PcdStatus);
> +      return;
>      default:
>        DEBUG ((DEBUG_ERROR, "%a: Unknown Host Bridge Device ID: 0x%04x\n",
>          __FUNCTION__, mHostBridgeDevId));
>        ASSERT (FALSE);
>        return;
>    }
> -  PcdStatus = PcdSet16S (PcdOvmfHostBridgePciDevId, mHostBridgeDevId);
> -  ASSERT_RETURN_ERROR (PcdStatus);

Removing this check looks suspicious.

If it is not needed here for some reason move that change to a separate
patch with a commit message explaining things.

take care,
  Gerd