[PATCH v4 0/1] hw/hyperv/vmbus: Is it maintained?

[PATCH v4 0/1] hw/hyperv/vmbus: Is it maintained?

[Philippe Mathieu-Daudé]

This is the 4th time I send this patch. Is the VMBus infrastructure
used / maintained? Should we deprecate & remove?

  $ ./scripts/get_maintainer.pl -f hw/hyperv/vmbus.c -f include/hw/hyperv/vmbus.h
  get_maintainer.pl: No maintainers found

Philippe Mathieu-Daudé (1):
  hw/hyperv/vmbus: Remove unused vmbus_load/save_req()

 include/hw/hyperv/vmbus.h |  3 --
 hw/hyperv/vmbus.c         | 59 ---------------------------------------
 2 files changed, 62 deletions(-)

-- 
2.31.1

[PATCH v4 1/1] hw/hyperv/vmbus: Remove unused vmbus_load/save_req()

[Philippe Mathieu-Daudé]

vmbus_save_req() and vmbus_load_req() are not used.
Remove them to avoid maintaining dead code.

This partially reverts commit 4dd8a7064b8a6527f99a62be11
("vmbus: add infrastructure to save/load vmbus requests").

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/hyperv/vmbus.h |  3 --
 hw/hyperv/vmbus.c         | 59 ---------------------------------------
 2 files changed, 62 deletions(-)

diff --git a/include/hw/hyperv/vmbus.h b/include/hw/hyperv/vmbus.h
index f98bea3888d..8ea660dd8e6 100644
--- a/include/hw/hyperv/vmbus.h
+++ b/include/hw/hyperv/vmbus.h
@@ -223,7 +223,4 @@ int vmbus_map_sgl(VMBusChanReq *req, DMADirection dir, struct iovec *iov,
 void vmbus_unmap_sgl(VMBusChanReq *req, DMADirection dir, struct iovec *iov,
                      unsigned iov_cnt, size_t accessed);
 
-void vmbus_save_req(QEMUFile *f, VMBusChanReq *req);
-void *vmbus_load_req(QEMUFile *f, VMBusDevice *dev, uint32_t size);
-
 #endif
diff --git a/hw/hyperv/vmbus.c b/hw/hyperv/vmbus.c
index dbce3b35fba..78c19caf5f7 100644
--- a/hw/hyperv/vmbus.c
+++ b/hw/hyperv/vmbus.c
@@ -1311,65 +1311,6 @@ static const VMStateDescription vmstate_vmbus_chan_req = {
     }
 };
 
-void vmbus_save_req(QEMUFile *f, VMBusChanReq *req)
-{
-    VMBusChanReqSave req_save;
-
-    req_save.chan_idx = req->chan->subchan_idx;
-    req_save.pkt_type = req->pkt_type;
-    req_save.msglen = req->msglen;
-    req_save.msg = req->msg;
-    req_save.transaction_id = req->transaction_id;
-    req_save.need_comp = req->need_comp;
-    req_save.num = req->sgl.nsg;
-    req_save.sgl = g_memdup(req->sgl.sg,
-                            req_save.num * sizeof(ScatterGatherEntry));
-
-    vmstate_save_state(f, &vmstate_vmbus_chan_req, &req_save, NULL);
-
-    g_free(req_save.sgl);
-}
-
-void *vmbus_load_req(QEMUFile *f, VMBusDevice *dev, uint32_t size)
-{
-    VMBusChanReqSave req_save;
-    VMBusChanReq *req = NULL;
-    VMBusChannel *chan = NULL;
-    uint32_t i;
-
-    vmstate_load_state(f, &vmstate_vmbus_chan_req, &req_save, 0);
-
-    if (req_save.chan_idx >= dev->num_channels) {
-        error_report("%s: %u(chan_idx) > %u(num_channels)", __func__,
-                     req_save.chan_idx, dev->num_channels);
-        goto out;
-    }
-    chan = &dev->channels[req_save.chan_idx];
-
-    if (vmbus_channel_reserve(chan, 0, req_save.msglen)) {
-        goto out;
-    }
-
-    req = vmbus_alloc_req(chan, size, req_save.pkt_type, req_save.msglen,
-                          req_save.transaction_id, req_save.need_comp);
-    if (req_save.msglen) {
-        memcpy(req->msg, req_save.msg, req_save.msglen);
-    }
-
-    for (i = 0; i < req_save.num; i++) {
-        qemu_sglist_add(&req->sgl, req_save.sgl[i].base, req_save.sgl[i].len);
-    }
-
-out:
-    if (req_save.msglen) {
-        g_free(req_save.msg);
-    }
-    if (req_save.num) {
-        g_free(req_save.sgl);
-    }
-    return req;
-}
-
 static void channel_event_cb(EventNotifier *e)
 {
     VMBusChannel *chan = container_of(e, VMBusChannel, notifier);
-- 
2.31.1

Re: [PATCH v4 0/1] hw/hyperv/vmbus: Is it maintained?

[Maciej S. Szmigiero]

On 06.11.2021 14:41, Philippe Mathieu-Daudé wrote:
> This is the 4th time I send this patch. Is the VMBus infrastructure
> used / maintained? Should we deprecate & remove?
> 
>    $ ./scripts/get_maintainer.pl -f hw/hyperv/vmbus.c -f include/hw/hyperv/vmbus.h
>    get_maintainer.pl: No maintainers found

There's an email thread at [1] explaining the reasons for having VMBus
infrastructure last time such question was asked.

In short: mere presence of a working VMBus is needed for some high-speed
Windows debugging, also people are working on VMBus host device drivers.

Your patch makes sense to me, so for it:
Reviewed-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>

Thanks,
Maciej

[1]: https://lore.kernel.org/qemu-devel/20201009193919.GF7303@habkost.net/T/#u

Re: [PATCH v4 0/1] hw/hyperv/vmbus: Is it maintained?

[Philippe Mathieu-Daudé]

On 11/6/21 20:28, Maciej S. Szmigiero wrote:
> On 06.11.2021 14:41, Philippe Mathieu-Daudé wrote:
>> This is the 4th time I send this patch. Is the VMBus infrastructure
>> used / maintained? Should we deprecate & remove?
>>
>>    $ ./scripts/get_maintainer.pl -f hw/hyperv/vmbus.c -f
>> include/hw/hyperv/vmbus.h
>>    get_maintainer.pl: No maintainers found
> 
> There's an email thread at [1] explaining the reasons for having VMBus
> infrastructure last time such question was asked.
> 
> In short: mere presence of a working VMBus is needed for some high-speed
> Windows debugging, also people are working on VMBus host device drivers.

Great. Do you mind adding an entry in MAINTAINERS to
cover these files, so we stop wondering about them?

> Your patch makes sense to me, so for it:
> Reviewed-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>

Thank you.

> [1]:
> https://lore.kernel.org/qemu-devel/20201009193919.GF7303@habkost.net/T/#u
> 

Re: [PATCH v4 0/1] hw/hyperv/vmbus: Is it maintained?

[Vladimir Sementsov-Ogievskiy]

Add Den and Roman (his new address)

06.11.2021 16:41, Philippe Mathieu-Daudé wrote:
> This is the 4th time I send this patch. Is the VMBus infrastructure
> used / maintained? Should we deprecate & remove?
> 
>    $ ./scripts/get_maintainer.pl -f hw/hyperv/vmbus.c -f include/hw/hyperv/vmbus.h
>    get_maintainer.pl: No maintainers found
> 
> Philippe Mathieu-Daudé (1):
>    hw/hyperv/vmbus: Remove unused vmbus_load/save_req()
> 
>   include/hw/hyperv/vmbus.h |  3 --
>   hw/hyperv/vmbus.c         | 59 ---------------------------------------
>   2 files changed, 62 deletions(-)
> 


-- 
Best regards,
Vladimir

Re: [PATCH v4 0/1] hw/hyperv/vmbus: Is it maintained?

[Roman Kagan]

On Fri, Nov 12, 2021 at 09:32:31PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Add Den and Roman (his new address)

Thanks, I missed it on the list indeed.

> 06.11.2021 16:41, Philippe Mathieu-Daudé wrote:
> > This is the 4th time I send this patch. Is the VMBus infrastructure
> > used / maintained? Should we deprecate & remove?

I think it's fair to say it's not maintained.  The whole
hw/hyperv/vmbus.c was submitted as a part of the work by Jon to enable
some obscure windows debugging feature which only worked in presence of
VMBus.  It was mostly taken from the respective branch of the (now
effectively abandoned) downstream tree with an implementation of the
core VMBus infrastructure and the devices using it; however, none of the
actual VMBus devices ever made it into the mainline tree.

> > 
> >    $ ./scripts/get_maintainer.pl -f hw/hyperv/vmbus.c -f include/hw/hyperv/vmbus.h
> >    get_maintainer.pl: No maintainers found
> > 
> > Philippe Mathieu-Daudé (1):
> >    hw/hyperv/vmbus: Remove unused vmbus_load/save_req()
> > 
> >   include/hw/hyperv/vmbus.h |  3 --
> >   hw/hyperv/vmbus.c         | 59 ---------------------------------------
> >   2 files changed, 62 deletions(-)

This seems to basically be the revert of 4dd8a7064b "vmbus: add
infrastructure to save/load vmbus requests"; it was originally meant to
be submitted with the code that would use it, vmbus scsi controller, but
that never happened.  It believe it's safe to remove without affecting
Jon's work, but I'd rather check with him.

Thanks,
Roman.

Re: [PATCH v4 0/1] hw/hyperv/vmbus: Is it maintained?

[Maciej S. Szmigiero]

On 12.11.2021 21:39, Roman Kagan wrote:
> On Fri, Nov 12, 2021 at 09:32:31PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Add Den and Roman (his new address)
> 
> Thanks, I missed it on the list indeed.
> 
>> 06.11.2021 16:41, Philippe Mathieu-Daudé wrote:
>>> This is the 4th time I send this patch. Is the VMBus infrastructure
>>> used / maintained? Should we deprecate & remove?
> 
> I think it's fair to say it's not maintained.  The whole
> hw/hyperv/vmbus.c was submitted as a part of the work by Jon to enable
> some obscure windows debugging feature which only worked in presence of
> VMBus.  It was mostly taken from the respective branch of the (now
> effectively abandoned) downstream tree with an implementation of the
> core VMBus infrastructure and the devices using it; however, none of the
> actual VMBus devices ever made it into the mainline tree.
> 

The VMBus code works fine, is mostly self-contained and by being a part
of the upstream QEMU it does benefit from any improvements done there and
so it is much less likely to bit-rot with time.

I am still committed to upstreaming a Hyper-V Dynamic Memory Protocol
driver (which uses VMBus), however had been preempted by higher-priority
work for now.

Thanks,
Maciej

Re: [PATCH v4 0/1] hw/hyperv/vmbus: Is it maintained?

[Maciej S. Szmigiero]

On 08.11.2021 08:30, Philippe Mathieu-Daudé wrote:
> On 11/6/21 20:28, Maciej S. Szmigiero wrote:
>> On 06.11.2021 14:41, Philippe Mathieu-Daudé wrote:
>>> This is the 4th time I send this patch. Is the VMBus infrastructure
>>> used / maintained? Should we deprecate & remove?
>>>
>>>     $ ./scripts/get_maintainer.pl -f hw/hyperv/vmbus.c -f
>>> include/hw/hyperv/vmbus.h
>>>     get_maintainer.pl: No maintainers found
>>
>> There's an email thread at [1] explaining the reasons for having VMBus
>> infrastructure last time such question was asked.
>>
>> In short: mere presence of a working VMBus is needed for some high-speed
>> Windows debugging, also people are working on VMBus host device drivers.
> 
> Great. Do you mind adding an entry in MAINTAINERS to
> cover these files, so we stop wondering about them?
> 

I will submit a patch next week adding myself as a reviewer of VMBus code
so there is at least some contact point for incoming patches.

We'll see whether the code still gets just a random patch a few times
a year or whether it requires a permanent maintainer to take care of it.

Thanks,
Maciej

Re: [PATCH v4 0/1] hw/hyperv/vmbus: Is it maintained?

[Jon Doron]

On 13/11/2021, Maciej S. Szmigiero wrote:
>On 12.11.2021 21:39, Roman Kagan wrote:
>>On Fri, Nov 12, 2021 at 09:32:31PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>Add Den and Roman (his new address)
>>
>>Thanks, I missed it on the list indeed.
>>
>>>06.11.2021 16:41, Philippe Mathieu-Daudé wrote:
>>>>This is the 4th time I send this patch. Is the VMBus infrastructure
>>>>used / maintained? Should we deprecate & remove?
>>
>>I think it's fair to say it's not maintained.  The whole
>>hw/hyperv/vmbus.c was submitted as a part of the work by Jon to enable
>>some obscure windows debugging feature which only worked in presence of
>>VMBus.  It was mostly taken from the respective branch of the (now
>>effectively abandoned) downstream tree with an implementation of the
>>core VMBus infrastructure and the devices using it; however, none of the
>>actual VMBus devices ever made it into the mainline tree.
>>
>
>The VMBus code works fine, is mostly self-contained and by being a part
>of the upstream QEMU it does benefit from any improvements done there and
>so it is much less likely to bit-rot with time.
>
>I am still committed to upstreaming a Hyper-V Dynamic Memory Protocol
>driver (which uses VMBus), however had been preempted by higher-priority
>work for now.
>
>Thanks,
>Maciej

Hi guys,

Sorry for the late reply, like Roman I also never got to submit the RFC 
for the Synth debugger device which requires the VMBus, I do hope to get 
to it at some point and VMBus is a required part for it.

In the last year or so I have not had much time to spend on this but I 
do hope to get back to finishing what I have started.

I'm not really sure I have the time or knowledge to maintain VMBus :( 
but I'll do my best to answer any questions as well.

-- Jon.