Re: [PATCH V1 09/16] arm/ioreq: Introduce arch specific bits for IOREQ/DM features

[Oleksandr <olekstysh@xxxxxxxxx>]

On 26.09.20 16:12, Julien Grall wrote:
> Hi,
Hi Julien.


> On 24/09/2020 19:02, Oleksandr wrote:
> > On 24.09.20 19:02, Oleksandr wrote:
> > > On 24.09.20 14:08, Jan Beulich wrote:
> > > > On 23.09.2020 22:16, Oleksandr wrote:
> > > > > On 23.09.20 21:03, Julien Grall wrote:
> > > > > > On 10/09/2020 21:22, Oleksandr Tyshchenko wrote:
> > > > > > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > > > > > > @@ -91,6 +108,28 @@ struct arch_domain
> > > > > > >    #endif
> > > > > > >    }  __cacheline_aligned;
> > > > > > >    +enum hvm_io_completion {
> > > > > > > +    HVMIO_no_completion,
> > > > > > > +    HVMIO_mmio_completion,
> > > > > > > +    HVMIO_pio_completion
> > > > > > > +};
> > > > > > > +
> > > > > > > +struct hvm_vcpu_io {
> > > > > > > +    /* I/O request in flight to device model. */
> > > > > > > +    enum hvm_io_completion io_completion;
> > > > > > > +    ioreq_t                io_req;
> > > > > > > +
> > > > > > > +    /*
> > > > > > > +     * HVM emulation:
> > > > > > > +     *  Linear address @mmio_gla maps to MMIO physical frame
> > > > > > > @mmio_gpfn.
> > > > > > > +     *  The latter is known to be an MMIO frame (not RAM).
> > > > > > > +     *  This translation is only valid for accesses as per
> > > > > > > @mmio_access.
> > > > > > > +     */
> > > > > > > +    struct npfec        mmio_access;
> > > > > > > +    unsigned long       mmio_gla;
> > > > > > > +    unsigned long       mmio_gpfn;
> > > > > > > +};
> > > > > > > +
> > > > > > Why do we need to re-define most of this? Can't this just be in 
> > > > > > common
> > > > > > code?
> > > > > Jan asked almost the similar question in "[PATCH V1 02/16] xen/ioreq:
> > > > > Make x86's IOREQ feature common".
> > > > > Please see my answer there:
> > > > > https://patchwork.kernel.org/patch/11769105/#23637511
> > > > >
> > > > > Theoretically we could move this to the common code, but the 
> > > > > question is
> > > > > how to be with other struct fields the x86's struct hvm_vcpu_io
> > > > > has/needs but
> > > > > Arm's seems not, would it be possible to logically split struct
> > > > > hvm_vcpu_io into common and arch parts?
> > > > Have struct vcpu_io and struct arch_vcpu_io as a sub-part of it?
> > > Although it is going to pull a lot of changes into x86/hvm/*, yes 
> > > this way
> > > we indeed could logically split struct hvm_vcpu_io into common and 
> > > arch parts in a clear way.
> > > If it is really worth it, I will start looking into it.
> > Julien, I noticed that three fields mmio* are not used within current 
> > series on Arm. Do we expect them to be used later on?
> IIRC, I just copied them blindly when writing the PoC.
>
> The information can already be found using the HSR (syndrome 
> register), so those fields would be redundant for us.
Got it.


> > Would be the following acceptable?
> > 1. Both fields "io_completion" and "io_req" (which seems to be the 
> > only fields used in common/ioreq.c) are moved to common struct vcpu 
> > as part of struct vcpu_io,
> >      enum hvm_io_completion is also moved (and renamed).
> > 2. We remove everything related to hvm_vcpu* from the Arm header.
> > 3. x86's struct hvm_vcpu_io stays as it is (but without two fields 
> > "io_completion" and "io_req").
> >      I think, this way we separate a common part and reduce Xen 
> > changes (which are getting bigger).
> The plan looks reasonable to me.
OK, will follow it. Thank you


--
Regards,

Oleksandr Tyshchenko