Re: [PATCH V2 02/23] xen/ioreq: Make x86's IOREQ feature common

[Oleksandr <olekstysh@xxxxxxxxx>]

On 17.11.20 17:29, Paul Durrant wrote:

Hi Paul

Thank you for the prompt answer.

> > > > The 'legacy' mechanism of mapping magic pages for ioreq servers
> > > > should remain x86 specific I think that aspect of the code needs to
> > > > remain behind and not get moved into common code. You could do that
> > > > in arch specific calls in hvm_ioreq_server_enable/disable() and
> > > > hvm_get_ioreq_server_info().
> > > Well, if legacy mechanism is not going to be used for other arch and
> > > should remain x86 specific, I will try to investigate what should be
> > > left in x86 code and rework the series.
> > > As a side note, I am afraid, we won't get a 100% code movement (which
> > > I managed to achieve here) for the next version of this patch as we
> > > need arch/x86/hvm/ioreq.c.
> > I am investigating how to split the code in order to leave the 'legacy'
> > mechanism x86 specific and have a few questions. Could you please
> > clarify the following:
> >
> > 1. The split of hvm_ioreq_server_enable/disable() is obvious to me, I
> > would like to clarify regarding hvm_get_ioreq_server_info().
> > Is it close to what you had in mind when suggesting the split of
> > hvm_get_ioreq_server_info() or I just need to abstract
> > hvm_ioreq_server_map_pages() call only?
> I think it is sufficient to just abstract hvm_ioreq_server_map_pages() (and 
> return -EOPNOTSUPP in the Arm case).
> The buf ioreq port should be common.
ok, will do.


> > (Not completed and non tested)
> >
> > +/* Called with ioreq_server lock held */
> > +int arch_ioreq_server_get_info(struct hvm_ioreq_server *s,
> > +                               unsigned long *ioreq_gfn,
> > +                               unsigned long *bufioreq_gfn,
> > +                               evtchn_port_t *bufioreq_port)
> > +{
> > +    if ( ioreq_gfn || bufioreq_gfn )
> > +    {
> > +        int rc = hvm_ioreq_server_map_pages(s);
> > +
> > +        if ( rc )
> > +            return rc;
> > +    }
> > +
> > +    if ( ioreq_gfn )
> > +        *ioreq_gfn = gfn_x(s->ioreq.gfn);
> > +
> > +    if ( HANDLE_BUFIOREQ(s) )
> > +    {
> > +        if ( bufioreq_gfn )
> > +            *bufioreq_gfn = gfn_x(s->bufioreq.gfn);
> > +
> > +        if ( bufioreq_port )
> > +            *bufioreq_port = s->bufioreq_evtchn;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >    int hvm_get_ioreq_server_info(struct domain *d, ioservid_t id,
> >                                  unsigned long *ioreq_gfn,
> >                                  unsigned long *bufioreq_gfn,
> > @@ -916,26 +954,7 @@ int hvm_get_ioreq_server_info(struct domain *d,
> > ioservid_t id,
> >        if ( s->emulator != current->domain )
> >            goto out;
> >
> > -    if ( ioreq_gfn || bufioreq_gfn )
> > -    {
> > -        rc = hvm_ioreq_server_map_pages(s);
> > -        if ( rc )
> > -            goto out;
> > -    }
> > -
> > -    if ( ioreq_gfn )
> > -        *ioreq_gfn = gfn_x(s->ioreq.gfn);
> > -
> > -    if ( HANDLE_BUFIOREQ(s) )
> > -    {
> > -        if ( bufioreq_gfn )
> > -            *bufioreq_gfn = gfn_x(s->bufioreq.gfn);
> > -
> > -        if ( bufioreq_port )
> > -            *bufioreq_port = s->bufioreq_evtchn;
> > -    }
> > -
> > -    rc = 0;
> > +    rc = arch_ioreq_server_get_info(s, ioreq_gfn, bufioreq_gfn,
> > bufioreq_port);
> >
> >     out:
> >        spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock);
> >
> > 2. If I understand the code correctly, besides of the above-mentioned
> > functions the arch specific calls should be in hvm_ioreq_server_init()
> > and hvm_ioreq_server_deinit() to actually hide
> > "hvm_ioreq_server_unmap_pages" usage from the common code.  I noticed
> > that the rollback code in hvm_ioreq_server_init() and the whole
> > hvm_ioreq_server_deinit() have a lot in common except an extra ASSERT()
> > and hvm_ioreq_server_free_pages() call in the latter. My question is
> > could we just replace the rollback code by hvm_ioreq_server_deinit()? I
> > assume an extra hvm_ioreq_server_free_pages() call wouldn't be an issue
> > here, but I am not sure what to do with the ASSERT, I expect it to be
> > triggered at such an early stage (so it probably needs moving out of the
> > hvm_ioreq_server_deinit() (or dropped?) as well as comment needs
> > updating). I am asking, because this way we would get *a single* arch
> > hook here which would be arch_ioreq_server_deinit() and remove code
> > duplication a bit.
> I would arch specific init and deinit, even if one of them does nothing... but 
> then I like symmetry :-)

Both hvm_ioreq_server_init() and hvm_ioreq_server_deinit() call "legacy" 
hvm_ioreq_server_unmap_pages()
which we want to be abstracted. The only difference between these two 
usages is that the former calls it during rollback only (in case of error).
Taking into the account what has been suggested for question #1 could we 
just introduce arch_ioreq_server_unmap_pages() to be called from both 
init and deinit?

[Not completed not tested]

@@ -762,7 +772,7 @@ static int hvm_ioreq_server_init(struct 
hvm_ioreq_server *s,
  fail_add:
     hvm_ioreq_server_remove_all_vcpus(s);
-    hvm_ioreq_server_unmap_pages(s);
+    arch_ioreq_server_unmap_pages(s);

     hvm_ioreq_server_free_rangesets(s);

@@ -776,7 +786,7 @@ static void hvm_ioreq_server_deinit(struct 
hvm_ioreq_server *s)
     hvm_ioreq_server_remove_all_vcpus(s);

     /*
-     * NOTE: It is safe to call both hvm_ioreq_server_unmap_pages() and
+     * NOTE: It is safe to call both arch_ioreq_server_unmap_pages() and
      *       hvm_ioreq_server_free_pages() in that order.
      *       This is because the former will do nothing if the pages
      *       are not mapped, leaving the page to be freed by the latter.
@@ -784,7 +794,7 @@ static void hvm_ioreq_server_deinit(struct 
hvm_ioreq_server *s)
      *       the page_info pointer to NULL, meaning the latter will do
      *       nothing.
      */
-    hvm_ioreq_server_unmap_pages(s);
+    arch_ioreq_server_unmap_pages(s);
     hvm_ioreq_server_free_pages(s);

     hvm_ioreq_server_free_rangesets(s);
@@ -918,7 +928,7 @@ int hvm_get_ioreq_server_info(struct domain *d, 
ioservid_t id,
     if ( ioreq_gfn || bufioreq_gfn )
     {
-        rc = hvm_ioreq_server_map_pages(s);
+        rc = arch_ioreq_server_map_pages(s);
         if ( rc )
             goto out;
     }


So looks like for leaving legacy mechanism x86 specific we need 4 new 
arch callbacks:
- arch_ioreq_server_enable
- arch_ioreq_server_disable
- arch_ioreq_server_map_pages
- arch_ioreq_server_unmap_pages
> > Something close to this.
> > (Not completed and non tested)
> >
> > @@ -761,18 +771,17 @@ static int hvm_ioreq_server_init(struct
> > hvm_ioreq_server *s,
> >        return 0;
> >
> >     fail_add:
> > -    hvm_ioreq_server_remove_all_vcpus(s);
> > -    hvm_ioreq_server_unmap_pages(s);
> > -
> > -    hvm_ioreq_server_free_rangesets(s);
> > -
> > -    put_domain(s->emulator);
> > +    hvm_ioreq_server_deinit(s);
> >        return rc;
> >    }
> >
> > +void arch_ioreq_server_deinit(struct hvm_ioreq_server *s)
> > +{
> > +    hvm_ioreq_server_unmap_pages(s);
> > +}
> > +
> >    static void hvm_ioreq_server_deinit(struct hvm_ioreq_server *s)
> >    {
> > -    ASSERT(!s->enabled);
> I assume this is the ASSERT you're referring to... There's no way we should be 
> deinit-ing an enabled server so that should remain in common code as is.
ok, I agree.

--
Regards,

Oleksandr Tyshchenko