Re: [Virtio-fs] [PATCH] vhost-user-fs: fix features handling

2021-04-10 Thread Anton Kuchin



On 09/04/2021 18:56, Vivek Goyal wrote:

On Thu, Apr 08, 2021 at 10:55:34PM +0300, Anton Kuchin wrote:

Make virtio-fs take into account server capabilities.

Just returning requested features assumes they all of then are implemented
by server and results in setting unsupported configuration if some of them
are absent.

Signed-off-by: Anton Kuchin

[CC stefan and qemu-devel.]

Can you give more details of what problem exactly you are facing. Or
this fix is about avoiding a future problem where device can refuse
to support a feature qemu is requesting for.


This fixes existing problem that qemu ignores features (un)supported by 
backend and this works fine only if backend features match features of 
qemu. Otherwise qemu incorrectly assumes that backend suports all of 
them and calls vhost_set_features() not taking into account result of 
previous vhost_get_features() call. This breaks protocol and can crash 
server or cause incorrect behavior.


This is why I hope it to be accepted in time for 6.0 release.


IIUC, this patch is preparing a list of features vhost-user-fs device
can support. Then it calls vhost_get_features() which makes sure that
all these features are support by real vhost-user device (hdev->features).
If not, then corresponding feature is reset and remaining features
are returned to caller.
When this callback is executed in virtio_bus_device_plugged() list of 
features that vhost-backend supports has been already obtained earlier 
by vhost_user_get_features() in vuf_device_realize() and stored in 
hdev->features. vuf_get_features() should return bitmask of features 
that are common for vhost backend (hdev->features) and frontend 
(vdev->host_features). But instead it just returns host features.

This feature negotion bit is called in so many places that I am kind of
lost that who should be doing what. Will leave it to Stefan who
understands it much better.



---
  hw/virtio/vhost-user-fs.c | 17 +
  1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
index ac4fc34b36..6cf983ba0e 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -24,6 +24,14 @@
  #include "monitor/monitor.h"
  #include "sysemu/sysemu.h"
  
+static const int user_feature_bits[] = {

+VIRTIO_F_VERSION_1,
+VIRTIO_RING_F_INDIRECT_DESC,
+VIRTIO_RING_F_EVENT_IDX,
+VIRTIO_F_NOTIFY_ON_EMPTY,
+VHOST_INVALID_FEATURE_BIT
+};
+
  static void vuf_get_config(VirtIODevice *vdev, uint8_t *config)
  {
  VHostUserFS *fs = VHOST_USER_FS(vdev);
@@ -129,11 +137,12 @@ static void vuf_set_status(VirtIODevice *vdev, uint8_t 
status)
  }
  
  static uint64_t vuf_get_features(VirtIODevice *vdev,

-  uint64_t requested_features,
-  Error **errp)
+ uint64_t features,

Will it make sense to keep the name requested_features. This kind of
makes it clear that caller is requesting these features.

I feel there should be few lines of comments also to make it clear
what this function is actually doing.


This fix was inspired by similar functions in 
hw/scsi/vhost-scsi-common.c, hw/virtio/vhost-user-vsock.c, 
hw/block/vhost-user-blk.c and hw/net/vhost_net.c and I borrowed naming 
from them just to be consistent.


IMO this looks like a good place for refactoring because we have more 
and more vhost-user devices that duplicate that code, but it can be done 
after the bug is fixed.



Vivek


+ Error **errp)
  {
-/* No feature bits used yet */
-return requested_features;
+VHostUserFS *fs = VHOST_USER_FS(vdev);
+
+return vhost_get_features(&fs->vhost_dev, user_feature_bits, features);
  }
  
  static void vuf_handle_output(VirtIODevice *vdev, VirtQueue *vq)

--
2.25.1


___
Virtio-fs mailing list
virtio...@redhat.com
https://listman.redhat.com/mailman/listinfo/virtio-fs




Re: [PATCH v2 5/8] target/riscv: Implementation of enhanced PMP (ePMP)

2021-04-10 Thread Alistair Francis
On Fri, Apr 9, 2021 at 10:33 AM Bin Meng  wrote:
>
> Hi Alistair,
>
> On Fri, Apr 9, 2021 at 8:23 PM Alistair Francis
>  wrote:
> >
> > From: Hou Weiying 
> >
> > This commit adds support for ePMP v0.9.1.
> >
> > The ePMP spec can be found in:
> > https://docs.google.com/document/d/1Mh_aiHYxemL0umN3GTTw8vsbmzHZ_nxZXgjgOUzbvc8
> >
> > Signed-off-by: Hongzheng-Li 
> > Signed-off-by: Hou Weiying 
> > Signed-off-by: Myriad-Dreamin 
> > Message-Id: 
> > 
> > [ Changes by AF:
> >  - Rebase on master
> >  - Update to latest spec
> >  - Use a switch case to handle ePMP MML permissions
> >  - Fix a few bugs
> > ]
> > Signed-off-by: Alistair Francis 
> > ---
> >  target/riscv/pmp.c | 165 +
> >  1 file changed, 153 insertions(+), 12 deletions(-)
> >
>
> It looks like the v1 comments are not addressed?

You are right. I sent this before I saw your comments for v1. I will
address those comments in a v3. Sorry about that.

Alistair

>
> Regards,
> Bin



[PATCH] target/riscv: fix wfi exception behavior

2021-04-10 Thread Jose Martins
The wfi exception trigger behavior was not taking into account the fact
that user mode is not allowed to execute wfi instructions or the effect
of the hstatus.vtw bit. It was also always generating virtual instruction
exceptions when this should only happen when the wfi instruction is
executed when the virtualization mode is enabled:

- when a wfi instruction is executed, an illegal exception should be triggered
if either the current mode is user or the mode is supervisor and mstatus.tw is
set.

- a virtual instruction exception should be raised when a wfi is executed from
virtual-user or virtual-supervisor and hstatus.vtw is set.

Signed-off-by: Jose Martins 
---
 target/riscv/cpu_bits.h  | 1 +
 target/riscv/op_helper.c | 8 +---
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 24b24c69c5..ed8b97c788 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -436,6 +436,7 @@
 #define HSTATUS_HU   0x0200
 #define HSTATUS_VGEIN0x0003F000
 #define HSTATUS_VTVM 0x0010
+#define HSTATUS_VTW  0x0020
 #define HSTATUS_VTSR 0x0040
 #if defined(TARGET_RISCV64)
 #define HSTATUS_VSXL0x3
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index d55def76cf..354e39ec26 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -174,9 +174,11 @@ void helper_wfi(CPURISCVState *env)
 {
 CPUState *cs = env_cpu(env);
 
-if ((env->priv == PRV_S &&
-get_field(env->mstatus, MSTATUS_TW)) ||
-riscv_cpu_virt_enabled(env)) {
+if ((!riscv_cpu_virt_enabled(env) && env->priv == PRV_U) ||
+(env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TW))) {
+riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
+} else if (riscv_cpu_virt_enabled(env) && (env->priv == PRV_U ||
+(env->priv == PRV_S && get_field(env->hstatus, HSTATUS_VTW {
 riscv_raise_exception(env, RISCV_EXCP_VIRT_INSTRUCTION_FAULT, GETPC());
 } else {
 cs->halted = 1;
-- 
2.25.1




Re: [PATCH 1/1] Set TARGET_PAGE_BITS to be 10 instead of 8 bits

2021-04-10 Thread Michael Rolnik
Please review.

On Tue, Mar 23, 2021 at 10:28 PM Michael Rolnik  wrote:

> If I set TARGET_PAGE_BITS to 12 this *assert assert(v_l2_levels >= 0);*
> will fail (page_table_config_init function) because
> TARGET_PHYS_ADDR_SPACE_BITS is 24 bits, because AVR has 24 is the longest
> pointer AVR has. I can set TARGET_PHYS_ADDR_SPACE_BITS to 32 and
> TARGET_PAGE_BITS to 12 and everything will work fine.
> What do you think?
>
> btw, wrote the original comment, you David referred to, when I did not
> know that QEMU could map several regions to the same page, which is not
> true. That's why I could change 8 to 10.
>
> On Tue, Mar 23, 2021 at 10:11 PM Michael Rolnik  wrote:
>
>> how long?
>>
>> On Tue, Mar 23, 2021 at 2:46 PM Dr. David Alan Gilbert <
>> dgilb...@redhat.com> wrote:
>>
>>> * Michael Rolnik (mrol...@gmail.com) wrote:
>>> > Signed-off-by: Michael Rolnik 
>>> > ---
>>> >  target/avr/cpu-param.h | 8 +---
>>> >  target/avr/helper.c| 2 --
>>> >  2 files changed, 1 insertion(+), 9 deletions(-)
>>> >
>>> > diff --git a/target/avr/cpu-param.h b/target/avr/cpu-param.h
>>> > index 7ef4e7c679..9765a9d0db 100644
>>> > --- a/target/avr/cpu-param.h
>>> > +++ b/target/avr/cpu-param.h
>>> > @@ -22,13 +22,7 @@
>>> >  #define AVR_CPU_PARAM_H
>>> >
>>> >  #define TARGET_LONG_BITS 32
>>> > -/*
>>> > - * TARGET_PAGE_BITS cannot be more than 8 bits because
>>> > - * 1.  all IO registers occupy [0x .. 0x00ff] address range, and
>>> they
>>> > - * should be implemented as a device and not memory
>>> > - * 2.  SRAM starts at the address 0x0100
>>>
>>> I don't know AVR; but that seems to say why you can't make it any larger
>>> - how do you solve that?
>>>
>>> Dave
>>>
>>> > -#define TARGET_PAGE_BITS 8
>>> > +#define TARGET_PAGE_BITS 10
>>> >  #define TARGET_PHYS_ADDR_SPACE_BITS 24
>>> >  #define TARGET_VIRT_ADDR_SPACE_BITS 24
>>> >  #define NB_MMU_MODES 2
>>> > diff --git a/target/avr/helper.c b/target/avr/helper.c
>>> > index 35e1019594..da658afed3 100644
>>> > --- a/target/avr/helper.c
>>> > +++ b/target/avr/helper.c
>>> > @@ -111,8 +111,6 @@ bool avr_cpu_tlb_fill(CPUState *cs, vaddr address,
>>> int size,
>>> >  MemTxAttrs attrs = {};
>>> >  uint32_t paddr;
>>> >
>>> > -address &= TARGET_PAGE_MASK;
>>> > -
>>> >  if (mmu_idx == MMU_CODE_IDX) {
>>> >  /* access to code in flash */
>>> >  paddr = OFFSET_CODE + address;
>>> > --
>>> > 2.25.1
>>> >
>>> --
>>> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
>>>
>>>
>>
>> --
>> Best Regards,
>> Michael Rolnik
>>
>
>
> --
> Best Regards,
> Michael Rolnik
>


-- 
Best Regards,
Michael Rolnik


Re: [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation

2021-04-10 Thread Philippe Mathieu-Daudé
+Mark & Igor

On 4/10/21 5:15 PM, Peter Maydell wrote:
> On Sat, 10 Apr 2021 at 14:53, Philippe Mathieu-Daudé  wrote:
>> On 4/10/21 3:19 PM, Luc Michel wrote:
>>> Note that clock propagation during reset has always been a complicated
>>> problem. Calling clock_propagate is forbidden during the reset's enter
>>> phase because of the side effects it can introduce.
>>
>> Ah... Maybe this is related to the generic reset problem in QEMU :(
> 
> I do wonder if we got the clock-propagation-during-reset part of this
> wrong -- it seemed right to me at the time but trying to use the
> clock API recently I did run into some unhelpful-seeming results
> (I forget the details now, though).
> 
>>> I find your API modification a bit restrictive. I think creating a
>>> standalone clock can be useful, e.g. in complicated devices that may
>>> want to use internal "intermediate" clocks. I would not remove this
>>> possibility to the API users.
>>
>> Well, this is the point. I can't see a justification to have a clock
>> on a non-qdev object. We should be able to model complicated devices
>> with qdev.
> 
> The obvious reason is that machine objects are not qdev devices (ie
> TYPE_MACHINE inherits directly from TYPE_OBJECT, not from TYPE_DEVICE),
> but it's a reasonable thing to say "this machine has a fixed frequency
> clock which it connects to the SoC".

In this series I restrict Clocks to 1/ qdev and 2/ MachineState (which
is the case you described). I tried to think about all the hardware I
worked with and all could be somehow modeled as qdev.

> I do wonder if the right fix to that would be to make TYPE_MACHINE
> be a subtype of TYPE_DEVICE, though -- machines not being subtypes
> of device has other annoying effects, like their not having reset
> methods or being able to register vmstate structs. There might be
> some unanticipated side effects of making that change, though.

Yes, that would simplify few things. I might try it.

Expanding the topic, this reminds me a discussion between Igor and Mark
about MemoryRegion...

One was about we can not change the NULL owner to MachineState
because the region could be migrated and there is a mismatch.

Another was about preparing the design for multi-arch machines, Mark was
confuse by having owner for memory regions such DRAM because on a board
they aren't owned, can be used by various masters (CPUs, DMA). So the
machine should be the owner somehow. Maybe the problem was with
migration indeed, I can't remember :S

Regards,

Phil.



Re: [PULL 00/10] Block layer fixes for 6.0-rc3

2021-04-10 Thread Peter Maydell
On Fri, 9 Apr 2021 at 17:16, Kevin Wolf  wrote:
>
> The following changes since commit ce69aa92d71e13db9c3702a8e8305e8d2463aeb8:
>
>   Merge remote-tracking branch 'remotes/jasowang/tags/net-pull-request' into 
> staging (2021-04-08 16:45:31 +0100)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to c2c731a4d35062295cd3260e66b3754588a2fad4:
>
>   test-blockjob: Test job_wait_unpaused() (2021-04-09 18:00:29 +0200)
>
> 
> Block layer fixes
>
> - mirror: Fix job-complete race condition causing unexpected errors
> - fdc: Fix 'fallback' property on sysbus floppy disk controllers
> - rbd: Fix memory leaks
> - iotest improvements


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/6.0
for any user-visible changes.

-- PMM



Re: [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation

2021-04-10 Thread Peter Maydell
On Sat, 10 Apr 2021 at 14:53, Philippe Mathieu-Daudé  wrote:
>
> Hi Luc,
>
> On 4/10/21 3:19 PM, Luc Michel wrote:
> > Note that clock propagation during reset has always been a complicated
> > problem. Calling clock_propagate is forbidden during the reset's enter
> > phase because of the side effects it can introduce.
>
> Ah... Maybe this is related to the generic reset problem in QEMU :(

I do wonder if we got the clock-propagation-during-reset part of this
wrong -- it seemed right to me at the time but trying to use the
clock API recently I did run into some unhelpful-seeming results
(I forget the details now, though).

> > I find your API modification a bit restrictive. I think creating a
> > standalone clock can be useful, e.g. in complicated devices that may
> > want to use internal "intermediate" clocks. I would not remove this
> > possibility to the API users.
>
> Well, this is the point. I can't see a justification to have a clock
> on a non-qdev object. We should be able to model complicated devices
> with qdev.

The obvious reason is that machine objects are not qdev devices (ie
TYPE_MACHINE inherits directly from TYPE_OBJECT, not from TYPE_DEVICE),
but it's a reasonable thing to say "this machine has a fixed frequency
clock which it connects to the SoC".

I do wonder if the right fix to that would be to make TYPE_MACHINE
be a subtype of TYPE_DEVICE, though -- machines not being subtypes
of device has other annoying effects, like their not having reset
methods or being able to register vmstate structs. There might be
some unanticipated side effects of making that change, though.

thanks
-- PMM



Re: [RFC PATCH-for-6.1 v2 6/6] hw/mips/loongson3_virt: Raise CPU clock to 2 GHz

2021-04-10 Thread Philippe Mathieu-Daudé
Hi Huacai,

On 4/10/21 4:43 AM, Huacai Chen wrote:
> Hi, Philippe,
> 
> On Fri, Apr 9, 2021 at 5:36 PM Philippe Mathieu-Daudé  wrote:
>>
>> Commit cd3a53b727d ("clock: Add clock_ns_to_ticks() function")
>> removed the limitation of using clock with a frequency of 1 GHz
>> or more.
>>
>> The previous commit converted the MIPS CP0 timer to use this
>> new clock_ns_to_ticks() function. We can now use a clock of
>> 2 GHz with the Loongson3 virt board.
> Yes, we can do this, but why should we do this? I don't think this can
> bring any advantages.

IIRC this was how you sent the earlier series, then we had to reduce
the frequency to <1GHz due to the DIV#0 bug. Now it is fixed I thought
you'd get this back.

I spent time with the R4K timer because it is often used by embedded
firmwares, and a mismatch with the CPU clock makes firmware not work
well. I suppose when using Linux guests it is not a real issue, because
1/ if there is another timer (different peripheral on a system-on-soc)
Linux will use it first, 2/ Linux does some early check to adapt with
the tick rate IIRC (it doesn't assume a precise rate).

I'm fine with the current Loongson3 virt board behavior with TCG,
so let's disregard this patch.

Thanks,

Phil.



Re: [PATCH for-6.0] esp: fix setting of ESPState mig_version_id when launching QEMU with -S option

2021-04-10 Thread Philippe Mathieu-Daudé
On 4/10/21 10:31 AM, Mark Cave-Ayland wrote:
> On 10/04/2021 06:56, Thomas Huth wrote:
> 
>> On 07/04/2021 14.48, Mark Cave-Ayland wrote:
>>> If QEMU is launched with the -S option then the ESPState
>>> mig_version_id property
>>> is left unset due to the ordering of the VMState fields in the
>>> VMStateDescription
>>> for sysbusespscsi and pciespscsi. If the VM is migrated and restored
>>> in this
>>> stopped state, the version tests in the vmstate_esp
>>> VMStateDescription and
>>> esp_post_load() become confused causing the migration to fail.
>>>
>>> Fix the ordering problem by moving the setting of mig_version_id to a
>>> common
>>> esp_pre_save() function which is invoked first by both sysbusespscsi and
>>> pciespscsi rather than at the point where ESPState is itself
>>> serialised into the
>>> migration stream.
>>>
>>> Buglink: https://bugs.launchpad.net/qemu/+bug/1922611
>>> Fixes: 0bd005be78 ("esp: add vmstate_esp version to embedded ESPState")
>>> Signed-off-by: Mark Cave-Ayland 
>>> ---
>>>   hw/scsi/esp-pci.c | 1 +
>>>   hw/scsi/esp.c | 7 ---
>>>   include/hw/scsi/esp.h | 1 +
>>>   3 files changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
>>> index c3d3dab05e..9db10b1a48 100644
>>> --- a/hw/scsi/esp-pci.c
>>> +++ b/hw/scsi/esp-pci.c
>>> @@ -332,6 +332,7 @@ static const VMStateDescription
>>> vmstate_esp_pci_scsi = {
>>>   .name = "pciespscsi",
>>>   .version_id = 2,
>>>   .minimum_version_id = 1,
>>> +    .pre_save = esp_pre_save,
>>>   .fields = (VMStateField[]) {
>>>   VMSTATE_PCI_DEVICE(parent_obj, PCIESPState),
>>>   VMSTATE_BUFFER_UNSAFE(dma_regs, PCIESPState, 0, 8 *
>>> sizeof(uint32_t)),
>>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>>> index 3b9037e4f4..0037197bdb 100644
>>> --- a/hw/scsi/esp.c
>>> +++ b/hw/scsi/esp.c
>>> @@ -1089,9 +1089,10 @@ static bool esp_is_version_5(void *opaque, int
>>> version_id)
>>>   return version_id == 5;
>>>   }
>>> -static int esp_pre_save(void *opaque)
>>> +int esp_pre_save(void *opaque)
>>>   {
>>> -    ESPState *s = ESP(opaque);
>>> +    ESPState *s = ESP(object_resolve_path_component(
>>> +  OBJECT(opaque), "esp"));
>>>   s->mig_version_id = vmstate_esp.version_id;
>>>   return 0;
>>> @@ -1127,7 +1128,6 @@ const VMStateDescription vmstate_esp = {
>>>   .name = "esp",
>>>   .version_id = 5,
>>>   .minimum_version_id = 3,
>>> -    .pre_save = esp_pre_save,
>>>   .post_load = esp_post_load,
>>>   .fields = (VMStateField[]) {
>>>   VMSTATE_BUFFER(rregs, ESPState),
>>> @@ -1317,6 +1317,7 @@ static const VMStateDescription
>>> vmstate_sysbus_esp_scsi = {
>>>   .name = "sysbusespscsi",
>>>   .version_id = 2,
>>>   .minimum_version_id = 1,
>>> +    .pre_save = esp_pre_save,
>>>   .fields = (VMStateField[]) {
>>>   VMSTATE_UINT8_V(esp.mig_version_id, SysBusESPState, 2),
>>>   VMSTATE_STRUCT(esp, SysBusESPState, 0, vmstate_esp, ESPState),
>>> diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
>>> index 95088490aa..aada3680b7 100644
>>> --- a/include/hw/scsi/esp.h
>>> +++ b/include/hw/scsi/esp.h
>>> @@ -157,5 +157,6 @@ void esp_hard_reset(ESPState *s);
>>>   uint64_t esp_reg_read(ESPState *s, uint32_t saddr);
>>>   void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t val);
>>>   extern const VMStateDescription vmstate_esp;
>>> +int esp_pre_save(void *opaque);
>>
>> Reviewed-by: Thomas Huth 
>>
>> Which tree should this patch go through? Your Sparc tree? Migration?
>> Scsi? Trivial?
> 
> Previously I've considered the ESP patches to be SCSI, although the
> large ESP patchset was given approval to go via another tree which is
> why that was eventually merged via qemu-sparc.
> 
> I don't mind doing the same again although I'm still waiting for the
> final nod for this and the ESP security fixes for 6.0 (see
> https://lists.gnu.org/archive/html/qemu-devel/2021-04/msg01479.html).
> 
> Thoughts/other ideas?

It makes sense to have this go via the SCSI tree, but the maintainers
are pretty busy (you forgot to Cc Fam in both series). Maybe with an Ack
from them you could take the ESP patches via the SPARC tree?

Regards,

Phil.



Re: [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation

2021-04-10 Thread Philippe Mathieu-Daudé
Hi Luc,

On 4/10/21 3:19 PM, Luc Michel wrote:
> On 08:23 Fri 09 Apr , Philippe Mathieu-Daudé wrote:
>> I've been debugging some odd issue with the clocks:
>> a clock created in the machine (IOW, not a qdev clock) isn't
>> always resetted, thus propagating its value.
>> "not always" is the odd part. In the MPS2 board, the machine
>> clock is propagated. Apparently because the peripherals are
>> created directly in the machine_init() handler. When moving
>> them out in a SoC QOM container, the clock isn't... I'm still
>> having hard time to understand what is going on.
> 
> I think there is a misunderstanding on how the clock API works. If I
> understand correctly your issue, you expect the callback of an input
> clock connected to your constant "main oscillator" clock to be called on
> machine reset.
> 
> If I'm not mistaken this is not the way the API has been designed. The
> callback is called only when the clock period changes. A constant clock
> does not change on reset, so the callback of child clocks should not be
> called.

They why the children of a clock tree fed with constant clock stay with
a clock of 0? Who is responsible of setting their clock to the constant
value?

> However devices that care about this clock value (e.g. a device that
> has a clock input connected to this constant clock) should see their
> standard reset callback called during reset. And they can effectively read
> the clock value here and do what they need to do.
> 
> Note that clock propagation during reset has always been a complicated
> problem. Calling clock_propagate is forbidden during the reset's enter
> phase because of the side effects it can introduce.

Ah... Maybe this is related to the generic reset problem in QEMU :(

>> Alternatively I tried to strengthen the clock API by reducing
>> the clock creation in 2 cases: machine/device. This way clocks
>> aren't left dangling around alone. The qdev clocks are properly
>> resetted, and for the machine clocks I register a generic reset
>> handler. This way is safer, but I don't think we want to keep
>> adding generic reset handlers, instead we'd like to remove them.
> 
> I find your API modification a bit restrictive. I think creating a
> standalone clock can be useful, e.g. in complicated devices that may
> want to use internal "intermediate" clocks. I would not remove this
> possibility to the API users.

Well, this is the point. I can't see a justification to have a clock
on a non-qdev object. We should be able to model complicated devices
with qdev.

We are having various problems with the CPUs which are non-qdev devices,
or recently even with the LED model...:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg798031.html

Phil.



Re: [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation

2021-04-10 Thread Luc Michel
Hi Philippe,

On 08:23 Fri 09 Apr , Philippe Mathieu-Daudé wrote:
> Hi Damian, Luc, Peter.
> 
> I've been debugging some odd issue with the clocks:
> a clock created in the machine (IOW, not a qdev clock) isn't
> always resetted, thus propagating its value.
> "not always" is the odd part. In the MPS2 board, the machine
> clock is propagated. Apparently because the peripherals are
> created directly in the machine_init() handler. When moving
> them out in a SoC QOM container, the clock isn't... I'm still
> having hard time to understand what is going on.

I think there is a misunderstanding on how the clock API works. If I
understand correctly your issue, you expect the callback of an input
clock connected to your constant "main oscillator" clock to be called on
machine reset.

If I'm not mistaken this is not the way the API has been designed. The
callback is called only when the clock period changes. A constant clock
does not change on reset, so the callback of child clocks should not be
called.

However devices that care about this clock value (e.g. a device that
has a clock input connected to this constant clock) should see their
standard reset callback called during reset. And they can effectively read
the clock value here and do what they need to do.

Note that clock propagation during reset has always been a complicated
problem. Calling clock_propagate is forbidden during the reset's enter
phase because of the side effects it can introduce.

> 
> Alternatively I tried to strengthen the clock API by reducing
> the clock creation in 2 cases: machine/device. This way clocks
> aren't left dangling around alone. The qdev clocks are properly
> resetted, and for the machine clocks I register a generic reset
> handler. This way is safer, but I don't think we want to keep
> adding generic reset handlers, instead we'd like to remove them.

I find your API modification a bit restrictive. I think creating a
standalone clock can be useful, e.g. in complicated devices that may
want to use internal "intermediate" clocks. I would not remove this
possibility to the API users.

Thanks,

-- 
Luc



Re: [PATCH for-6.0 2/2] block/nbd: ensure ->connection_thread is always valid

2021-04-10 Thread Roman Kagan
On Sat, Apr 10, 2021 at 12:56:34PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 10.04.2021 11:38, Vladimir Sementsov-Ogievskiy wrote:
> > 10.04.2021 11:06, Vladimir Sementsov-Ogievskiy wrote:
> > > 09.04.2021 19:04, Roman Kagan wrote:
> > > > Simplify lifetime management of BDRVNBDState->connection_thread by
> > > > delaying the possible cleanup of it until the BDRVNBDState itself goes
> > > > away.
> > > > 
> > > > This also fixes possible use-after-free in nbd_co_establish_connection
> > > > when it races with nbd_co_establish_connection_cancel.
> > > > 
> > > > Signed-off-by: Roman Kagan
> > > 
> > > Reviewed-by: Vladimir Sementsov-Ogievskiy 
> > > 
> > 
> > Ha stop, it crashes iotest 51, as nbd_clear_bdrvstate is called also from 
> > nbd_process_options.
> > 
> > And this shows that we also do wrong thing when simply return from two ifs 
> > pre-patch (and one after-patch). Yes, after successful nbd_process options 
> > we should call nbd_clear_bdrvstate() on failure path.

The test didn't crash at me somehow but you're right there's bug here.

> And also it shows that patch is more difficult than it seems. I still think 
> that for 6.0 we should take a simple use-after-free-only fix, and don't care 
> about anything else.

I agree it turned out more complex than apporpriate for 6.0.  Let's
proceed with the one you've posted.

Thanks,
Roman.



Re: Mac OS real USB device support issue

2021-04-10 Thread BALATON Zoltan

On Sat, 10 Apr 2021, Howard Spoelstra wrote:

On Fri, Apr 9, 2021 at 9:37 PM Programmingkid  wrote:

Have you tried the proposed changes yet for libusb?


Hi,

Yes, I experimented with the current libusb from brew, the latest
libusb code from github and a patched version based on that. I
couldn't get a flash drive passed through with any of them. Running as
root made no difference. My Mojave host doesn't allow unloading the
kext loaded for the flash drive where Sierra allowed that, but then
that should be handled by the patches.

I'll link to the latest libusb and the patched version plus the
patches. I guess it will not work on your host, but you might be able
to persuade qemu to use them by using
install_name_tool -change /usr/local/opt/libusb/lib/libusb-1.0.0.dylib
@executable_path/libusb-1.0.0-latest.dylib qemu-system-ppc

I'll also include the patches, libusb is easily built.
https://surfdrive.surf.nl/files/index.php/s/Qs0rtTVe2qIudw4/download


I think you (John and Gerd) found that detecting if a kernel driver is 
attached does not seem to work so it does not even get to unloading what 
these patches are about I think. So you first need to debug and fix 
libusb_kernel_driver_active() so the unloading function is called at all. 
I don't know how that's done on macOS but maybe querying the IO registry 
somehow that should have all info about devices and IOKit drivers.


Regards,
BALATON Zoltan



Re: [PATCH for-6.0 2/2] block/nbd: ensure ->connection_thread is always valid

2021-04-10 Thread Vladimir Sementsov-Ogievskiy

10.04.2021 11:38, Vladimir Sementsov-Ogievskiy wrote:

10.04.2021 11:06, Vladimir Sementsov-Ogievskiy wrote:

09.04.2021 19:04, Roman Kagan wrote:

Simplify lifetime management of BDRVNBDState->connection_thread by
delaying the possible cleanup of it until the BDRVNBDState itself goes
away.

This also fixes possible use-after-free in nbd_co_establish_connection
when it races with nbd_co_establish_connection_cancel.

Signed-off-by: Roman Kagan


Reviewed-by: Vladimir Sementsov-Ogievskiy 



Ha stop, it crashes iotest 51, as nbd_clear_bdrvstate is called also from 
nbd_process_options.

And this shows that we also do wrong thing when simply return from two ifs 
pre-patch (and one after-patch). Yes, after successful nbd_process options we 
should call nbd_clear_bdrvstate() on failure path.



And also it shows that patch is more difficult than it seems. I still think 
that for 6.0 we should take a simple use-after-free-only fix, and don't care 
about anything else.

--
Best regards,
Vladimir



Re: [RFC PATCH 5/5] sockets: Support multipath TCP

2021-04-10 Thread Markus Armbruster
Daniel P. Berrangé  writes:

> On Thu, Apr 08, 2021 at 08:11:59PM +0100, Dr. David Alan Gilbert (git) wrote:
>> From: "Dr. David Alan Gilbert" 
>> 
>> Multipath TCP allows combining multiple interfaces/routes into a single
>> socket, with very little work for the user/admin.
>> 
>> It's enabled by 'mptcp' on most socket addresses:
>> 
>>./qemu-system-x86_64 -nographic -incoming tcp:0:,mptcp
>> 
>> Signed-off-by: Dr. David Alan Gilbert 
>> ---
>>  io/dns-resolver.c   |  2 ++
>>  qapi/sockets.json   |  5 -
>>  util/qemu-sockets.c | 34 ++
>>  3 files changed, 40 insertions(+), 1 deletion(-)
>> 
>> diff --git a/io/dns-resolver.c b/io/dns-resolver.c
>> index 743a0efc87..b081e098bb 100644
>> --- a/io/dns-resolver.c
>> +++ b/io/dns-resolver.c
>> @@ -122,6 +122,8 @@ static int 
>> qio_dns_resolver_lookup_sync_inet(QIODNSResolver *resolver,
>>  .ipv4 = iaddr->ipv4,
>>  .has_ipv6 = iaddr->has_ipv6,
>>  .ipv6 = iaddr->ipv6,
>> +.has_mptcp = iaddr->has_mptcp,
>> +.mptcp = iaddr->mptcp,
>>  };
>>  
>>  (*addrs)[i] = newaddr;
>> diff --git a/qapi/sockets.json b/qapi/sockets.json
>> index 2e83452797..43122a38bf 100644
>> --- a/qapi/sockets.json
>> +++ b/qapi/sockets.json
>> @@ -57,6 +57,8 @@
>>  # @keep-alive: enable keep-alive when connecting to this socket. Not 
>> supported
>>  #  for passive sockets. (Since 4.2)
>>  #
>> +# @mptcp: enable multi-path TCP. (Since 6.0)
>> +#
>>  # Since: 1.3
>>  ##
>>  { 'struct': 'InetSocketAddress',
>> @@ -66,7 +68,8 @@
>>  '*to': 'uint16',
>>  '*ipv4': 'bool',
>>  '*ipv6': 'bool',
>> -'*keep-alive': 'bool' } }
>> +'*keep-alive': 'bool',
>> +'*mptcp': 'bool' } }
>
> I think this would need to be
>
>'*mptcp': { 'type': 'bool', 'if': 'IPPROTO_MPTCP' }
>
> so that mgmt apps can probe when it truely is supported or not for
> this build

Yes.  Instance of a somewhat common anti-pattern "declare
unconditionally (this hunk), write unconditionally (previous hunk), read
conditionally (next hunk).  Besides defeating introspection, it also
exposes configuration knobs that don't do anything.

>>  
>>  ##
>>  # @UnixSocketAddress:
>> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
>> index 8af0278f15..72527972d5 100644
>> --- a/util/qemu-sockets.c
>> +++ b/util/qemu-sockets.c
>> @@ -206,6 +206,21 @@ static int try_bind(int socket, InetSocketAddress 
>> *saddr, struct addrinfo *e)
>>  #endif
>>  }
>>  
>> +static int check_mptcp(const InetSocketAddress *saddr, struct addrinfo *ai,
>> +   Error **errp)
>> +{
>> +if (saddr->has_mptcp && saddr->mptcp) {
>> +#ifdef IPPROTO_MPTCP
>> +ai->ai_protocol = IPPROTO_MPTCP;
>> +#else
>> +error_setg(errp, "MPTCP unavailable in this build");
>> +return -1;
>> +#endif
>> +}
>> +
>> +return 0;
>> +}
>> +
>>  static int inet_listen_saddr(InetSocketAddress *saddr,
>>   int port_offset,
>>   int num,
[...]




Re: gitlab-ci check-dco test

2021-04-10 Thread Philippe Mathieu-Daudé
On 4/10/21 4:58 AM, Richard Henderson wrote:
> On development branches, it's not uncommon to push
> temporary --fixup patches, and normally one doesn't
> sign those.  But then of course one get hate-mail
> from the gitlab-ci job about the failing test.
> 
> Is there a way to make it fatal on staging, but
> merely a warning on other branches (a-la checkpatch)?

To only run check-dco on branch /staging on any namespace:

-- >8 --
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 3480d79db3a..f0d21da57f0 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -781,9 +781,9 @@ check-dco:
   needs:
 job: amd64-centos8-container
   script: .gitlab-ci.d/check-dco.py
-  except:
+  only:
 variables:
-  - $CI_PROJECT_NAMESPACE == 'qemu-project' && $CI_COMMIT_BRANCH ==
'master'
+  - $CI_COMMIT_BRANCH == 'staging'
   variables:
 GIT_DEPTH: 1000

---



Re: [PATCH 1/2] qdev: Separate implementations of qdev_get_machine() for user and system

2021-04-10 Thread Markus Armbruster
Greg Kurz  writes:

> Despite its simple name and common usage of "getting a pointer to
> the machine" in system-mode emulation, qdev_get_machine() has some
> subtilities.
>
> First, it can be called when running user-mode emulation : this is
> because user-mode partly relies on qdev to instantiate its CPU
> model.
>
> Second, but not least, it has a side-effect : if it cannot find an
> object at "/machine" in the QOM tree, it creates a dummy "container"
> object and put it there. A simple check on the type returned by
> qdev_get_machine() allows user-mode to run the common qdev code,
> skipping the parts that only make sense for system-mode.
>
> This side-effect turns out to complicate the use of qdev_get_machine()
> for the system-mode case though. Most notably, qdev_get_machine() must
> not be called before the machine object is added to the QOM tree by
> qemu_create_machine(), otherwise the existing dummy "container" object
> would cause qemu_create_machine() to fail with something like :

Stupid trap.

> Unexpected error in object_property_try_add() at ../../qom/object.c:1223:
> qemu-system-ppc64: attempt to add duplicate property 'machine' to
>  object (type 'container')
> Aborted (core dumped)
>
> This situation doesn't exist in the current code base, mostly because
> of preventive fixing of some "latent bugs" in QEMU 4.0 (see 1a3ec8c1564
> and e2fb3fbbf9c for details).

I lacked the stamina to address the root problem: automatic creation of
dummy containers where real ones may be needed.

Is /machine the only such container?  Have you reviewed the other uses
of container_get()?

> A new kind of breakage was spotted very recently though :
>
> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> /home/thuth/devel/qemu/include/hw/boards.h:24:
>  MACHINE: Object 0x5635bd53af10 is not an instance of type machine
> Aborted (core dumped)
>
> This comes from the change 3df261b6676b in QEMU 5.0. It unwillingly
> added a new condition for qdev_get_machine() to be called too early,
> breaking MACHINE(qdev_get_machine()) in generic cpu-core code this
> time.
>
> In order to avoid further subtle breakages like this, change the
> implentation of qdev_get_machine() to:
> - keep the existing behaviour of creating the dummy "container"
>   object for the user-mode case only ;
> - abort() if the machine doesn't exist yet in the QOM tree for
>   the system-mode case. This gives a precise hint to developpers
>   that calling qdev_get_machine() too early is a programming bug.

In other words, we fail right away instead of planting a landmine for
later.  Good.

The alternative would be mandating "must create /machine before first
use" for all programs, not just qemu-system-FOO, but that might be more
invasive.  Not sure.

> This is achieved with a new do_qdev_get_machine() function called

container_get() is a suboptimal name for a function that creates
containers, qdev_get_machine() is a suboptimal name for a function that
creates /machine, and so is do_qdev_get_machine().  Observation, not
demand.

> from qdev_get_machine(), with different implementations for system
> and user mode.
>
> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> qemu-system-ppc64: ../../hw/core/machine.c:1290:
>  qdev_get_machine: Assertion `machine != NULL' failed.
> Aborted (core dumped)
>
> Reported-by: Thomas Huth 
> Signed-off-by: Greg Kurz 




Re: [PATCH for-6.0 2/2] block/nbd: ensure ->connection_thread is always valid

2021-04-10 Thread Vladimir Sementsov-Ogievskiy

10.04.2021 11:06, Vladimir Sementsov-Ogievskiy wrote:

09.04.2021 19:04, Roman Kagan wrote:

Simplify lifetime management of BDRVNBDState->connection_thread by
delaying the possible cleanup of it until the BDRVNBDState itself goes
away.

This also fixes possible use-after-free in nbd_co_establish_connection
when it races with nbd_co_establish_connection_cancel.

Signed-off-by: Roman Kagan


Reviewed-by: Vladimir Sementsov-Ogievskiy 



Ha stop, it crashes iotest 51, as nbd_clear_bdrvstate is called also from 
nbd_process_options.

And this shows that we also do wrong thing when simply return from two ifs 
pre-patch (and one after-patch). Yes, after successful nbd_process options we 
should call nbd_clear_bdrvstate() on failure path.

--
Best regards,
Vladimir



Re: [PATCH for-6.0] esp: fix setting of ESPState mig_version_id when launching QEMU with -S option

2021-04-10 Thread Mark Cave-Ayland

On 10/04/2021 06:56, Thomas Huth wrote:


On 07/04/2021 14.48, Mark Cave-Ayland wrote:

If QEMU is launched with the -S option then the ESPState mig_version_id property
is left unset due to the ordering of the VMState fields in the 
VMStateDescription
for sysbusespscsi and pciespscsi. If the VM is migrated and restored in this
stopped state, the version tests in the vmstate_esp VMStateDescription and
esp_post_load() become confused causing the migration to fail.

Fix the ordering problem by moving the setting of mig_version_id to a common
esp_pre_save() function which is invoked first by both sysbusespscsi and
pciespscsi rather than at the point where ESPState is itself serialised into the
migration stream.

Buglink: https://bugs.launchpad.net/qemu/+bug/1922611
Fixes: 0bd005be78 ("esp: add vmstate_esp version to embedded ESPState")
Signed-off-by: Mark Cave-Ayland 
---
  hw/scsi/esp-pci.c | 1 +
  hw/scsi/esp.c | 7 ---
  include/hw/scsi/esp.h | 1 +
  3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
index c3d3dab05e..9db10b1a48 100644
--- a/hw/scsi/esp-pci.c
+++ b/hw/scsi/esp-pci.c
@@ -332,6 +332,7 @@ static const VMStateDescription vmstate_esp_pci_scsi = {
  .name = "pciespscsi",
  .version_id = 2,
  .minimum_version_id = 1,
+    .pre_save = esp_pre_save,
  .fields = (VMStateField[]) {
  VMSTATE_PCI_DEVICE(parent_obj, PCIESPState),
  VMSTATE_BUFFER_UNSAFE(dma_regs, PCIESPState, 0, 8 * sizeof(uint32_t)),
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 3b9037e4f4..0037197bdb 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -1089,9 +1089,10 @@ static bool esp_is_version_5(void *opaque, int 
version_id)
  return version_id == 5;
  }
-static int esp_pre_save(void *opaque)
+int esp_pre_save(void *opaque)
  {
-    ESPState *s = ESP(opaque);
+    ESPState *s = ESP(object_resolve_path_component(
+  OBJECT(opaque), "esp"));
  s->mig_version_id = vmstate_esp.version_id;
  return 0;
@@ -1127,7 +1128,6 @@ const VMStateDescription vmstate_esp = {
  .name = "esp",
  .version_id = 5,
  .minimum_version_id = 3,
-    .pre_save = esp_pre_save,
  .post_load = esp_post_load,
  .fields = (VMStateField[]) {
  VMSTATE_BUFFER(rregs, ESPState),
@@ -1317,6 +1317,7 @@ static const VMStateDescription vmstate_sysbus_esp_scsi = 
{
  .name = "sysbusespscsi",
  .version_id = 2,
  .minimum_version_id = 1,
+    .pre_save = esp_pre_save,
  .fields = (VMStateField[]) {
  VMSTATE_UINT8_V(esp.mig_version_id, SysBusESPState, 2),
  VMSTATE_STRUCT(esp, SysBusESPState, 0, vmstate_esp, ESPState),
diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
index 95088490aa..aada3680b7 100644
--- a/include/hw/scsi/esp.h
+++ b/include/hw/scsi/esp.h
@@ -157,5 +157,6 @@ void esp_hard_reset(ESPState *s);
  uint64_t esp_reg_read(ESPState *s, uint32_t saddr);
  void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t val);
  extern const VMStateDescription vmstate_esp;
+int esp_pre_save(void *opaque);


Reviewed-by: Thomas Huth 

Which tree should this patch go through? Your Sparc tree? Migration? Scsi? 
Trivial?


Previously I've considered the ESP patches to be SCSI, although the large ESP 
patchset was given approval to go via another tree which is why that was eventually 
merged via qemu-sparc.


I don't mind doing the same again although I'm still waiting for the final nod for 
this and the ESP security fixes for 6.0 (see 
https://lists.gnu.org/archive/html/qemu-devel/2021-04/msg01479.html).


Thoughts/other ideas?


ATB,

Mark.



[PATCH] accel/tcg: Logs num_insns in translate_block trace events.

2021-04-10 Thread Nathan Ringo
This makes it easier to figure out whether a particular instruction was
actually translated.

Signed-off-by: Nathan Ringo 
---
 accel/tcg/trace-events| 2 +-
 accel/tcg/translate-all.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/accel/tcg/trace-events b/accel/tcg/trace-events
index 6eefb37f5d..c227e56248 100644
--- a/accel/tcg/trace-events
+++ b/accel/tcg/trace-events
@@ -7,4 +7,4 @@ exec_tb_nocache(void *tb, uintptr_t pc) "tb:%p pc=0x%"PRIxPTR
 exec_tb_exit(void *last_tb, unsigned int flags) "tb:%p flags=0x%x"
 
 # translate-all.c
-translate_block(void *tb, uintptr_t pc, const void *tb_code) "tb:%p, 
pc:0x%"PRIxPTR", tb_code:%p"
+translate_block(void *tb, uintptr_t pc, int num_insns, const void *tb_code) 
"tb:%p, pc:0x%"PRIxPTR", num_insns:%d, tb_code:%p"
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index f32df8b240..d43936b9b4 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -1916,7 +1916,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
 tcg_ctx->cpu = NULL;
 max_insns = tb->icount;
 
-trace_translate_block(tb, tb->pc, tb->tc.ptr);
+trace_translate_block(tb, tb->pc, (int)tb->icount, tb->tc.ptr);
 
 /* generate machine code */
 tb->jmp_reset_offset[0] = TB_JMP_RESET_OFFSET_INVALID;
-- 
2.26.3





Re: [PATCH for-6.0 2/2] block/nbd: ensure ->connection_thread is always valid

2021-04-10 Thread Vladimir Sementsov-Ogievskiy

09.04.2021 19:04, Roman Kagan wrote:

Simplify lifetime management of BDRVNBDState->connection_thread by
delaying the possible cleanup of it until the BDRVNBDState itself goes
away.

This also fixes possible use-after-free in nbd_co_establish_connection
when it races with nbd_co_establish_connection_cancel.

Signed-off-by: Roman Kagan


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir



Re: [PATCH for-6.0 1/2] block/nbd: fix channel object leak

2021-04-10 Thread Vladimir Sementsov-Ogievskiy

09.04.2021 19:04, Roman Kagan wrote:

nbd_free_connect_thread leaks the channel object if it hasn't been
stolen.

Unref it and fix the leak.

Signed-off-by: Roman Kagan


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir



Re: Mac OS real USB device support issue

2021-04-10 Thread Howard Spoelstra
On Fri, Apr 9, 2021 at 9:37 PM Programmingkid  wrote:
>
>
>
> > On Apr 7, 2021, at 1:28 AM, Howard Spoelstra  wrote:
> >
> > On Wed, Apr 7, 2021 at 7:26 AM Howard Spoelstra  wrote:
> >>
> >> On Wed, Apr 7, 2021 at 3:53 AM Programmingkid  
> >> wrote:
> >>>
> >>>
> >>>
>  On Apr 6, 2021, at 7:18 PM, BALATON Zoltan  wrote:
> 
>  On Tue, 6 Apr 2021, Programmingkid wrote:
> >> On Apr 6, 2021, at 12:53 PM, BALATON Zoltan  wrote:
> >> On Tue, 6 Apr 2021, Programmingkid wrote:
>  On Apr 6, 2021, at 10:01 AM, Howard Spoelstra  
>  wrote:
>  On Tue, Apr 6, 2021 at 3:44 PM Programmingkid 
>   wrote:
> >
> > Hi Gerd,
> >
> > I was wondering if you had access to a Mac OS 10 or Mac OS 11 
> > machine to test USB support. I am on Mac OS 11.1 and cannot make 
> > USB devices work with any of my guests. So far these are the guests 
> > I have tested with:
> >
> > - Windows 7
> > - Mac OS 9.2
> > - Windows 2000
> >
> > I have tried using USB flash drives, USB sound cards, and an USB 
> > headset. They all show up under 'info usb', but cannot be used in 
> > the guest. My setup does use a USB-C hub so I'm not sure if this is 
> > a bug with QEMU or an issue with the hub. Would you have any 
> > information on this issue?
> 
>  Hi John,
> 
>  As far as the Mac OS 9.2 guest is concerned on a mac OS host, it does
>  not support USB 2.0. I was successful only in passing through a USB
>  flash drive that was forced into USB 1.1 mode by connecting it to a
>  real USB 1.1 hub and unloading the kext it used.
> 
>  Best,
>  Howard
> >>>
> >>> Hi Howard, I was actually thinking about CC'ing you for this email. 
> >>> Glad you found it. Unloading kext files does not sound pleasant. 
> >>> Maybe there is some better way of doing it.
> >>
> >> In any case, until you make sure nothing tries to drive the device on 
> >> the host, passing it to a guest likely will fail because then two 
> >> drivers from two OSes would try to access it simultaneously which 
> >> likely creates a mess as the device and drivers don't expect this. So 
> >> you can't just pass a device through that the host has recognised and 
> >> is driving without somehow getting the host to leave it alone first 
> >> before you can pass it through. Unloading the driver is one way to do 
> >> that (although it probably breaks all other similar devices too). 
> >> Maybe there's another way to unbind a device from the host such as 
> >> ejecting it first but then I'm not sure if the low level USB needed 
> >> for accessing the device still works after that or it's completely 
> >> forgotten. There's probably a doc somewhere that describes how it 
> >> works and how can you plug a device without also getting higher level 
> >> drivers to load or if there's no official ways for that then you'll 
> >> need to do some configuration on the host t
>  o avoid it grabbing devices that you want to pass through. On Linux you 
>  can add an udev rule to ignore the device (maybe also adding 
>  TAG+="uaccess" to allow console users to use it without needing root 
>  access) but not sure how USB works on macOS.
> >>
> >> Regards,
> >> BALATON Zoltan
> >
> > Being able to dissociate a real USB device from its Mac OS driver would 
> > be very useful in this situation. IOKit might be one place to look for 
> > such a feature. The Mach kernel documentation is another place that 
> > might have what we want.
> 
>  Those might be a good place to start. IOKit provides the drivers and 
>  also the io registry which is probably where you can get if a driver is 
>  bound to a device and which one is it. How to dissociate the driver from 
>  the device though I don't know.
> >>>
> >>> https://developer.apple.com/library/archive/documentation/DeviceDrivers/Conceptual/IOKitFundamentals/DeviceRemoval/DeviceRemoval.html
> >>> According to this article a driver has a stop() and detach() method that 
> >>> is called by the IOKit to remove a device. I'm thinking QEMU can be the 
> >>> one that calls these methods for a certain device.
> >>>
> 
> > I have one theory. What if we introduce a middleman. A pseudo-USB 
> > device that the guest operating system could apply its configuration 
> > data to and will also talk directly with to the real USB device.
> > So this:
> >
> > USB device <-> Host <-> QEMU USB middleman <-> Guest
> 
>  Isn't this middleman the QEMU usb-host device that we already have?
> >>>
> >>> It could be. I need to research this issue some more.
> >>>
> 
> > This could make USB 2.0 and 3.0 flash drives compatible with an older 
> > o

Re: [PATCH 15/24] aspeed/smc: Add extra controls to request DMA

2021-04-10 Thread Cédric Le Goater
On 4/9/21 8:54 AM, Joel Stanley wrote:
> On Wed, 7 Apr 2021 at 17:17, Cédric Le Goater  wrote:
>>
>> The AST2600 SPI controllers have a set of bits to request/grant DMA
>> access. Add a new SMC feature for these controllers and use it to
>> check access to the DMA registers.
> 
> Ah this is why you added the features mask. Makes sense.

Yes. It's a bit redundant with the dma_ctrl() handler but it looks cleaner. 

> 
> Reviewed-by: Joel Stanley 


Thanks,

C.


>>
>> Cc: Chin-Ting Kuo 
>> Signed-off-by: Cédric Le Goater 
>> ---
>>  include/hw/ssi/aspeed_smc.h |  1 +
>>  hw/ssi/aspeed_smc.c | 74 +
>>  2 files changed, 68 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
>> index 07879fd1c4a7..cdaf165300b6 100644
>> --- a/include/hw/ssi/aspeed_smc.h
>> +++ b/include/hw/ssi/aspeed_smc.h
>> @@ -55,6 +55,7 @@ typedef struct AspeedSMCController {
>> const AspeedSegments *seg);
>>  void (*reg_to_segment)(const struct AspeedSMCState *s, uint32_t reg,
>> AspeedSegments *seg);
>> +void (*dma_ctrl)(struct AspeedSMCState *s, uint32_t value);
>>  } AspeedSMCController;
>>
>>  typedef struct AspeedSMCFlash {
>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
>> index 4521bbd4864e..189b35637c77 100644
>> --- a/hw/ssi/aspeed_smc.c
>> +++ b/hw/ssi/aspeed_smc.c
>> @@ -127,6 +127,8 @@
>>
>>  /* DMA Control/Status Register */
>>  #define R_DMA_CTRL(0x80 / 4)
>> +#define   DMA_CTRL_REQUEST  (1 << 31)
>> +#define   DMA_CTRL_GRANT(1 << 30)
>>  #define   DMA_CTRL_DELAY_MASK   0xf
>>  #define   DMA_CTRL_DELAY_SHIFT  8
>>  #define   DMA_CTRL_FREQ_MASK0xf
>> @@ -228,6 +230,7 @@ static uint32_t aspeed_smc_segment_to_reg(const 
>> AspeedSMCState *s,
>>const AspeedSegments *seg);
>>  static void aspeed_smc_reg_to_segment(const AspeedSMCState *s, uint32_t reg,
>>AspeedSegments *seg);
>> +static void aspeed_smc_dma_ctrl(AspeedSMCState *s, uint32_t value);
>>
>>  /*
>>   * AST2600 definitions
>> @@ -257,7 +260,10 @@ static uint32_t aspeed_2600_smc_segment_to_reg(const 
>> AspeedSMCState *s,
>> const AspeedSegments *seg);
>>  static void aspeed_2600_smc_reg_to_segment(const AspeedSMCState *s,
>> uint32_t reg, AspeedSegments 
>> *seg);
>> +static void aspeed_2600_smc_dma_ctrl(AspeedSMCState *s, uint32_t value);
>> +
>>  #define ASPEED_SMC_FEATURE_DMA   0x1
>> +#define ASPEED_SMC_FEATURE_DMA_GRANT 0x2
>>
>>  static inline bool aspeed_smc_has_dma(const AspeedSMCState *s)
>>  {
>> @@ -281,6 +287,7 @@ static const AspeedSMCController controllers[] = {
>>  .nregs = ASPEED_SMC_R_SMC_MAX,
>>  .segment_to_reg= aspeed_smc_segment_to_reg,
>>  .reg_to_segment= aspeed_smc_reg_to_segment,
>> +.dma_ctrl  = aspeed_smc_dma_ctrl,
>>  }, {
>>  .name  = "aspeed.fmc-ast2400",
>>  .r_conf= R_CONF,
>> @@ -299,6 +306,7 @@ static const AspeedSMCController controllers[] = {
>>  .nregs = ASPEED_SMC_R_MAX,
>>  .segment_to_reg= aspeed_smc_segment_to_reg,
>>  .reg_to_segment= aspeed_smc_reg_to_segment,
>> +.dma_ctrl  = aspeed_smc_dma_ctrl,
>>  }, {
>>  .name  = "aspeed.spi1-ast2400",
>>  .r_conf= R_SPI_CONF,
>> @@ -315,6 +323,7 @@ static const AspeedSMCController controllers[] = {
>>  .nregs = ASPEED_SMC_R_SPI_MAX,
>>  .segment_to_reg= aspeed_smc_segment_to_reg,
>>  .reg_to_segment= aspeed_smc_reg_to_segment,
>> +.dma_ctrl  = aspeed_smc_dma_ctrl,
>>  }, {
>>  .name  = "aspeed.fmc-ast2500",
>>  .r_conf= R_CONF,
>> @@ -333,6 +342,7 @@ static const AspeedSMCController controllers[] = {
>>  .nregs = ASPEED_SMC_R_MAX,
>>  .segment_to_reg= aspeed_smc_segment_to_reg,
>>  .reg_to_segment= aspeed_smc_reg_to_segment,
>> +.dma_ctrl  = aspeed_smc_dma_ctrl,
>>  }, {
>>  .name  = "aspeed.spi1-ast2500",
>>  .r_conf= R_CONF,
>> @@ -349,6 +359,7 @@ static const AspeedSMCController controllers[] = {
>>  .nregs = ASPEED_SMC_R_MAX,
>>  .segment_to_reg= aspeed_smc_segment_to_reg,
>>  .reg_to_segment= aspeed_smc_reg_to_segment,
>> +.dma_ctrl  = aspeed_smc_dma_ctrl,
>>  }, {
>>  .name  = "aspeed.spi2-ast2500",
>>  .r_conf= R_CONF,
>> @@ -365,6 +376,7 @@ static const AspeedSMCController controllers[] = {
>>  .nregs = ASPEED_SMC_R_MAX,
>>  .segment_to_reg= aspeed_smc_segment_to_re