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

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

On Wed, May 07, 2025 at 09:38:51AM -0400, Stewart Hildebrand wrote:
> On 5/7/25 03:44, Roger Pau Monné wrote:
> > On Tue, May 06, 2025 at 11:05:13PM -0400, Stewart Hildebrand wrote:
> >> On 5/6/25 07:16, Roger Pau Monné wrote:
> >>> On Fri, Apr 18, 2025 at 02:58:37PM -0400, Stewart Hildebrand wrote:
> >>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> >>>>  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.
> 
> The impression I get from [0] is that that on Arm, there's no benefit to
> performing truncation in xen/arch/arm/io.c. Doing so would needlessly
> affect other Arm internal read handlers (e.g. vGIC).

But isn't this truncation desirable in order to avoid possibly setting
bits outside of the access size?

> For vPCI
> specifically, however, we could potentially perform truncation in
> xen/arch/arm/vpci.c. So I guess it's a question of whether we want to
> give special treatment to vPCI compared to all other read handlers on
> Arm?

I would think doing the truncation uniformly for all reads would be
better, as we then ensure the value propagated to the registers always
matches the access size?

I'm not expert on ARM, but it seems cumbersome to force this to all
internal handlers, instead of just truncating the value in a single
place.

> >> 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,
> 
> OK
> 
> > and it should have been part of
> > 9a5e22b64266 I think, since that's the commit that introduced the
> > checking.
> 
> If we proceed with adjusting vpci_read() and vpci_read_hw(): are you OK
> with these adjustments included in this patch, or would you prefer them
> being split into a pre-patch?

Ideally it should be a separate patch with a "Fixes:" tag that points
to the patch that added the ASSERT().  As I think the current vPCI code is
kind of broken without that truncation on ARM?

Thanks, Roger.