Re: [PATCH v4 3/6] x86/PVH: permit more physdevop-s to be used by Dom0

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Sep 29, 2021 at 03:14:13PM +0200, Jan Beulich wrote:
> Certain notifications of Dom0 to Xen are independent of the mode Dom0 is
> running in. Permit further PCI related ones (only their modern forms).
> Also include the USB2 debug port operation at this occasion.

Sorry, I realize now that I failed to provide a reply on the previous
patch.

> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> I'm uncertain about the has_vpci() part of the check: I would think
> is_hardware_domain() is both sufficient and concise. Without vPCI a PVH
> Dom0 won't see any PCI devices in the first place (and hence would
> effectively be non-functioning). Dropping this would in particular make
> PHYSDEVOP_dbgp_op better fit in the mix.

I agree, it's not an option to have a non-vPCI PVH dom0 anyway, and
the important restriction for those operations is the hardware domain
part.

> ---
> v3: New.
> 
> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -94,6 +94,12 @@ static long hvm_physdev_op(int cmd, XEN_
>          break;
>  
>      case PHYSDEVOP_pci_mmcfg_reserved:
> +    case PHYSDEVOP_pci_device_add:
> +    case PHYSDEVOP_pci_device_remove:

Those are indeed fine.

> +    case PHYSDEVOP_restore_msi_ext:

While I understand the current need for this one in order to possibly
restore the MSI interrupt for the serial console, it might be a
cleaner approach to let dom0 restore the PCI state on it's own using
the native methods like it's done for initial setup.

I realize we are missing a bunch of stuff here, for once vPCI state
should be reset and put in sync with the hardware values after a
restore from suspension.

And then we likely need to either limit PHYSDEVOP_restore_msi_ext to
just restore Xen used MSI vectors, or if technically possible it would
be best from Xen to setup it's own MSI vectors when restoring without
relying on dom0.

> +    case PHYSDEVOP_dbgp_op:

This one is also fine.

> +    case PHYSDEVOP_prepare_msix:
> +    case PHYSDEVOP_release_msix:

Those two again I'm not sure should be added right now, as we still
haven't decided how pci passthrough will work from the hardware domain
PoV. I'm explicitly concerned about those two because they will mess
with the MSI-X state behind the back of vPCI, and nothing good will
came out of it.

If we really need those two in the future, code should be added so
that vPCI state is properly updated.

Thanks, Roger.