Re: [Xen-devel] [PATCH v2] vpci: honor read-only devices

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.09.2019 17:30, Roger Pau Monne wrote:
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -418,13 +418,21 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, 
> unsigned int size,
>          return;
>      }
>  
> -    /*
> -     * Find the PCI dev matching the address.
> -     * Passthrough everything that's not trapped.
> -     */
> +    /* Find the PCI dev matching the address. */
>      pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn);
>      if ( !pdev )
>      {
> +        const unsigned long *ro_map = pci_get_ro_map(sbdf.seg);
> +
> +        if ( ro_map && test_bit(sbdf.bdf, ro_map) )
> +            /* Ignore writes to read-only devices. */
> +            return;
> +
> +        /*
> +         * Let the hardware domain access config space regions for 
> non-existent
> +         * devices.
> +         * TODO: revisit for domU support.
> +         */
>          vpci_write_hw(sbdf, reg, size, data);
>          return;
>      }
> 

In principle I'm okay with the change, but I have two more things
to be considered:

1) I'd prefer if the check was independent of the return value of
pci_get_pdev_by_domain(), to be more robust against the r/o map
having got updated but the owner still being hwdom.

2) Wouldn't it be better to move the check into the callers of
vpci_write(), to avoid the duplicate lookup in the qword-MCFG-
write case? The main questionable point here is where, for DomU
support, the SBDF translation is going to live.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel