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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 5/8/25 04:32, Roger Pau Monné wrote:
> 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?

Yes, will do