Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features

[Oleksandr <olekstysh@xxxxxxxxx>]

On 21.08.20 09:16, Jan Beulich wrote:

Hi Jan.

Thank you for your answer.

> On 20.08.2020 20:30, Oleksandr wrote:
> > On 06.08.20 14:29, Jan Beulich wrote:
> > > On 06.08.2020 13:08, Julien Grall wrote:
> > > > On 05/08/2020 20:30, Oleksandr wrote:
> > > > > I was thinking how to split handle_hvm_io_completion()
> > > > > gracefully but I failed find a good solution for that, so decided to add
> > > > > two stubs (msix_write_completion and handle_realmode_completion) on Arm.
> > > > > I could add a comment describing why they are here if appropriate. But
> > > > > if you think they shouldn't be called from the common code in any way, I
> > > > > will try to split it.
> > > > I am not entirely sure what msix_write_completion is meant to do on x86.
> > > > Is it dealing with virtual MSIx? Maybe Jan, Roger or Paul could help?
> > > Due to the split brain model of handling PCI pass-through (between
> > > Xen and qemu), a guest writing to an MSI-X entry needs this write
> > > handed to qemu, and upon completion of the write there Xen also
> > > needs to take some extra action.
> > 1. Regarding common handle_hvm_io_completion() implementation:
> >
> > Could msix_write_completion() be called later on so we would be able to
> > split handle_hvm_io_completion() gracefully or could we call it from
> > handle_mmio()?
> > The reason I am asking is to avoid calling it from the common code in
> > order to avoid introducing stub on Arm which is not going to be ever
> > implemented
> > (if msix_write_completion() is purely x86 material).
> I'm unconvinced of this last fact, but as with about everything it is
> quite certainly possible to call the function later. The question is
> how ugly this would become, as this may involve redundant conditionals
> (i.e. ones which need to remain in sync) and/or extra propagation of
> state.

I understand. Would it be better to make handle_hvm_io_completion() per 
arch then?
This would avoid using various stubs on Arm (we could get rid of 
has_vpci, msix_write_completion, handle_pio, 
arch_handle_hvm_io_completion, etc)
and avoid renaming of handle_mmio().

Julien what is your opinion on that?


For example the Arm implementation would look like:

bool handle_hvm_io_completion(struct vcpu *v)
{
    struct domain *d = v->domain;
    struct hvm_vcpu_io *vio = &v->arch.hvm.hvm_io;
    struct hvm_ioreq_server *s;
    struct hvm_ioreq_vcpu *sv;
    enum hvm_io_completion io_completion;

    sv = get_pending_vcpu(v, &s);
    if ( sv && !hvm_wait_for_io(sv, get_ioreq(s, v)) )
        return false;

    vio->io_req.state = hvm_ioreq_needs_completion(&vio->io_req) ?
        STATE_IORESP_READY : STATE_IOREQ_NONE;

    vcpu_end_shutdown_deferral(v);

    io_completion = vio->io_completion;
    vio->io_completion = HVMIO_no_completion;

    switch ( io_completion )
    {
    case HVMIO_no_completion:
        break;

    case HVMIO_mmio_completion:
        return ioreq_handle_complete_mmio();

    default:
        ASSERT_UNREACHABLE();
        break;
    }

    return true;
}


> > 2. Regarding renaming common handle_mmio() to ioreq_handle_complete_mmio():
> >
> > There was a request to consider renaming that function which is called
> > from the common code in the context of IOREQ series.
> > The point is, that the name of the function is pretty generic and can be
> > confusing on Arm (we already have a try_handle_mmio()).
> > I noticed that except common code that function is called from a few
> > places on x86 (I am not even sure whether all of them are IOREQ related).
> > The question is would x86 folks be happy with such renaming?
> handle_mmio() without any parameters and used for a varying set
> of purposes was imo never a good choice of name. The situation
> has improved, but can do with further improvement. The new name,
> if to be used for truly renaming the function need to fit all
> uses though. As such, I don't think ioreq_handle_complete_mmio()
> is an appropriate name.
>
> > Alternatively I could provide the following in
> > include/asm-arm/hvm/ioreq.h without renaming it in the common code and
> > still using non-confusing variant on Arm (however I am not sure whether
> > this is a good idea):
> >
> > #define handle_mmio ioreq_handle_complete_mmio
> If anything, for x86 it ought to be the other way around, at
> which point you wouldn't need any alias #define on Arm.
But could this approach be accepted? I think it would be the easiest way
to avoid confusing on Arm and avoid renaming that function in the whole 
x86 code.

> > 3. Regarding common IOREQ/DM stuff location:
> >
> > Currently it is located at:
> > common/hvm/...
> > include/xen/hvm/...
> >
> > For the non-RFC patch series I am going to avoid using "hvm" name (which
> > is internal detail of arch specific code and shouldn't be exposed to the
> > common code).
> > The question is whether I should use another directory name (probably
> > ioreq?) or just place them in common root directory?
> I think there are arguments for and against hvm/. I'm not of
> the opinion that ioreq/ would be a good name, so if hvm/ was to
> be ruled out, I think the file(s) shouldn't go into separate
> subdirs at all.
Got it.


--
Regards,

Oleksandr Tyshchenko