Re: [PATCH v20 2/2] vpci: translate virtual PCI bus topology for guests

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, May 06, 2025 at 11:05:13PM -0400, Stewart Hildebrand wrote:
> On 5/6/25 07:16, Roger Pau Monné wrote:
> > Hello,
> > 
> > Sorry I haven't looked at this before, I was confused with the cover
> > letter having ARM in the subject and somehow assumed all the code was
> > ARM related.
> 
> No worries, thanks for taking a look.
> 
> > On Fri, Apr 18, 2025 at 02:58:37PM -0400, Stewart Hildebrand wrote:
> >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> >>  static int vpci_register_cmp(const struct vpci_register *r1,
> >>                               const struct vpci_register *r2)
> >>  {
> >> @@ -438,7 +473,7 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size)
> >>      const struct pci_dev *pdev;
> >>      const struct vpci_register *r;
> >>      unsigned int data_offset = 0;
> >> -    uint32_t data = ~(uint32_t)0;
> >> +    uint32_t data = 0xffffffffU >> (32 - 8 * size);
> > 
> > This seems kind of unrelated to the rest of the code in the patch,
> > why is this needed?  Isn't it always fine to return all ones, and let
> > the caller truncate to the required size?
> > 
> > Otherwise the code in vpci_read_hw() also needs to be adjusted.
> 
> On Arm, since 9a5e22b64266 ("xen/arm: check read handler behavior") we
> assert that the read handlers don't set any bits above the access size.

I see.  That kind of diverges from x86 behavior, that AFAICT (see
memcpy() at tail of hvmemul_do_io()) instead truncates the memcpy to
the size of the access.

Maybe it would be better to instead of asserting just truncate the
returned value to the given size, as that would allow to just return
~0 from handlers without having to care about the specific access
size.

> I had adjusted data here due to returning it directly from vpci_read()
> in the current form of the patch. With your suggestion below we would
> only need to adjust vpci_read_hw() (and then data here would not
> strictly need adjusting).

Both returns would need adjusting IMO, and it should have been part of
9a5e22b64266 I think, since that's the commit that introduced the
checking.

> 
> > And
> > you can likely use GENMASK(size * 8, 0) as it's easier to read.
> 
> OK. I'll then also provide a definition for GENMASK in
> tools/tests/vpci/emul.h.
> 
> >>  
> >>      if ( !size )
> >>      {
> >> @@ -453,9 +488,21 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size)
> >>       * pci_lock is sufficient.
> >>       */
> >>      read_lock(&d->pci_lock);
> >> -    pdev = pci_get_pdev(d, sbdf);
> >> -    if ( !pdev && is_hardware_domain(d) )
> >> -        pdev = pci_get_pdev(dom_xen, sbdf);
> >> +    if ( is_hardware_domain(d) )
> >> +    {
> >> +        pdev = pci_get_pdev(d, sbdf);
> >> +        if ( !pdev )
> >> +            pdev = pci_get_pdev(dom_xen, sbdf);
> >> +    }
> >> +    else
> >> +    {
> >> +        pdev = translate_virtual_device(d, &sbdf);
> >> +        if ( !pdev )
> >> +        {
> >> +            read_unlock(&d->pci_lock);
> >> +            return data;
> >> +        }
> > 
> > You don't need this checking here, as the check below will already be
> > enough AFAICT, iow:
> > 
> >     if ( is_hardware_domain(d) )
> >     {
> >         pdev = pci_get_pdev(d, sbdf);
> >         if ( !pdev )
> >             pdev = pci_get_pdev(dom_xen, sbdf);
> >     }
> >     else
> >         pdev = translate_virtual_device(d, &sbdf);
> > 
> >     if ( !pdev || !pdev->vpci )
> >     {
> >         read_unlock(&d->pci_lock);
> >         return vpci_read_hw(sbdf, reg, size);
> >     }
> > 
> > Achieves the same and is more compact by having a single return for
> > all the possible cases?  Same for the write case below.
> 
> Seeing as there is a is_hardware_domain check inside vpci_read_hw(),
> that is okay. I'll make the adjustment here and in vpci_write.

Yup, vpci_read_hw() is already prepared to handle calls from
!is_hardware_domain() contexts.

Thanks, Roger.