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 05:17:58PM -0400, Stewart Hildebrand wrote:
> On 5/7/25 13:44, Roger Pau Monné wrote:
> > 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?
> 
> On Arm we expect the read handlers to have the bits above the access
> size zeroed. If a read handler sets bits above the access size, it could
> indicate a bug in the read handler. As a reminder, this was already
> discussed at [0] and a patch was already committed 9a5e22b64266
> ("xen/arm: check read handler behavior"). Perhaps we could both keep the
> ASSERT (for debug builds) and perform truncation (for release builds) in
> xen/arch/arm/io.c:handle_read(), but that's patch for another day.
> 
> [0] 
> https://lore.kernel.org/xen-devel/20240522225927.77398-1-stewart.hildebrand@xxxxxxx/T/#t

Oh, I see.  I already expressed concerns on that thread about forcing
the truncation to be done by handler implementations vs truncating in
a generic place ahead of propagating to the registers.

My main concern is when returning ~0, as it seems cumbersome to have
to truncate that, and I think we do blindly return ~0 on more than one
x86 IO handler.

> >> 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.
> 
> To move this forward, I suggest performing this truncation in
> xen/arch/arm/vpci.c:vpci_mmio_read(). This will be a single place to
> perform truncation for Arm vPCI, and will not affect other Arm internal
> mmio handlers.

You already have the mask there, so it should be easy to do:

*r = data & invalid;

To truncate the value.  Could you send that as a separate patch with a
Fixes tag?

Thanks, I'm sorry for not realizing about this before.

Roger.